Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth/aws: Allow binding by EC2 instance IDs #3816

Merged
merged 23 commits into from
Mar 15, 2018

Conversation

joelthompson
Copy link
Contributor

@joelthompson joelthompson commented Jan 19, 2018

This allows specifying a list of EC2 instance IDs that are allowed to
bind to the role. To keep style formatting with the other bindings, this
is still called bound_ec2_instance_id rather than bound_ec2_instance_ids.

Partially fixes #3797

This allows specifying a list of EC2 instance IDs that are allowed to
bind to the role. To keep style formatting with the other bindings, this
is still called bound_ec2_instance_id rather than bound_ec2_instance_ids
as I intend to convert the other bindings to accept lists as well (where
it makes sense) and keeping them with singular names would be the
easiest for backwards compatibility.

Partially fixes hashicorp#3797
@joelthompson
Copy link
Contributor Author

I realize that some of the decisions I made when writing this probably seem a bit questionable at first blush, so I wanted to explain them.

First, I think that eventually we'll want to convert most if not all binds to being TypeCommaStringSplice instead of TypeString. But I'm not sure of the best way to do that -- do we want to create a bunch of duplicate fields in awsRoleEntry, one for singular and one for plural (e.g., BoundAmiId of type string AND BoundAmiIds of type []string)? If we do that, we'd have to pay a one-time upgrade cost on each role and be done with it. But, without more code, it would break backwards compatibility with the API on reading (as the field names would change), and so we might have to pay the cost to re-build a comma-separated string every time the role is read. The second option is to just always split on comma upon reading from storage and re-join on comma whenever writing to storage. There's somewhat higher overhead cost to this, but it maintains backwards compatibility everywhere. I tend to favor the second option, but not sure.

Since I wasn't sure what to do with my first question, and because I wanted to get something out there to solve the request in #3797, I figured I'd try to just address that one request.

So my second decision point was, do I only support a single EC2 instance ID, or do I support multiple from the beginning? I decided on multiple because that's the direction I want to go eventually, and I don't want to half-deliver a feature to @rorybrowne that he needs to hack around.

Third, do I call it bound_ec2_instance_id (singular) or bound_ec2_instance_ids (plural)? Plural seems right, but if I make more bindings accept comma-separated lists (my first point), I'd need to either rename all of them to have plural parameter names (which requires extra effort to maintain backwards compatibility) or just keep the singular names and have them accept multiple values. Again, I think the latter is probably the right call, and if I'm going to do that, then I think the bound_ec2_instance_id parameter name should be singular just to maintain consistency with all other bindings.

Anyway, would love feedback on any of these points, happy to adjust as y'all see fit!

@joelthompson
Copy link
Contributor Author

Also, it looks like Travis is failing for reasons not related to this PR, and running the aws auth backend acceptance tests worked for me when running locally.

@jefferai jefferai added this to the 0.9.3 milestone Jan 19, 2018
@jefferai jefferai modified the milestones: 0.9.3, 0.9.4 Jan 28, 2018
In the aws auth method, allow a number of binds to take in lists
instead of a single string value. The intended semantic is that, for
each bind type set, clients must match at least one of each of the bind
types set in order to authenticate.
@jefferai
Copy link
Member

jefferai commented Feb 5, 2018

Hi Joel,

Decision 1: We've been slowly making this exact breaking change across the API for the sake of better consistency. So if converting to TypeCommaStringSlice, which I encourage, then yeah -- pay the one-time upgrade cost, and have the returned value come from the new role entry, and have it be returned as an array. For people that want to configure things programmatically, it's way better to have it support array in -> array out. One thing to note about the upgrades: they need to be protected. See https://github.com/hashicorp/vault/blob/master/builtin/logical/pki/path_roles.go#L319

Decision 2: Sounds good

Decision 3: I agree that plural sounds right but given that the whole backend uses singular, I guess just keep it singular. If we decide to fix this up across all of the parameters we can change this one as well, but in the meantime it keeps things consistent within the same role.

@joelthompson
Copy link
Contributor Author

@jefferai -- since I've gotten to it in the meantime, I'd actually like to get #3907 merged first as it adds in the broader list support, which solve some of these questions. And ack on the upgrade guard, I've added it to #3907

@joelthompson joelthompson mentioned this pull request Feb 6, 2018
2 tasks
@jefferai jefferai modified the milestones: 0.9.4, 0.10 Feb 14, 2018
@jefferai jefferai modified the milestones: 0.10, 0.9.5 Feb 21, 2018
Acceptance tests were failing due to hashicorp#4014 so, as a workaround for now,
passing in the identity document and the RSA signature rather than the
PKCS7 document.
This reverts commit c342691.

The underlying issue causing the need for the workaround has been fixed,
and additional testing changes have been added in hashicorp#4031 so this is no
longer necessary.
Looks like these changes were dropped from prior commit
@jefferai jefferai modified the milestones: 0.9.5, 0.9.6 Feb 28, 2018
@joelthompson
Copy link
Contributor Author

Sorry the commit history is such a mess, but this should be ready for review! :)

I tried (and hope I succeeded!) to keep the singular and plural names consistent with #3907 now that that's merged.

@vishalnayak vishalnayak self-assigned this Mar 5, 2018
@@ -647,6 +658,13 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request
numBinds++
}

if len(roleEntry.BoundEc2InstanceIDs) > 0 {
if !allowEc2Binds {
return logical.ErrorResponse(fmt.Sprintf("specified bound_ec2_instance_id but not allowing ec2 auth_type or inferring %s", ec2EntityType)), nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This language is slightly awkward in that you don't "allow" ec2 auth type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little awkward, but that's exactly what all the other error messages say. I've updated everywhere to use specifying instead of allowing, I think that's more accurate.

@@ -384,6 +384,11 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context,
return nil, fmt.Errorf("nil identityDoc")
}

// Verify that the instance ID matches one of the ones set by the role
if len(roleEntry.BoundEc2InstanceIDs) > 0 && !strutil.StrListContains(roleEntry.BoundEc2InstanceIDs, *instance.InstanceId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you explicitly dereference the pointer here and in the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dereference it here so I don't get a compiler error:

$ make test TEST=./builtin/credential/aws/
go generate 
# github.com/hashicorp/vault/builtin/credential/aws
/.../go/src/github.com/hashicorp/vault/builtin/credential/aws/path_login.go:388:111: cannot use instance.InstanceId (type *string) as type string in argument to strutil.StrListContains
FAIL    github.com/hashicorp/vault/builtin/credential/aws [build failed]
Makefile:34: recipe for target 'test' failed
make: *** [test] Error 2

I dereference it in the next line so that it shows up as expected in the error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, forgot about how it's all pointers in AWS land. Sounds good!

@jefferai
Copy link
Member

Two minor comments, LGTM otherwise!

"allowing" is a bit of an awkward word, changing it to specifying to
be more accurate.
@jefferai jefferai merged commit d349f5b into hashicorp:master Mar 15, 2018
@jefferai
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support AWS EC2 Auth Bound By Instance ID and Security Group
3 participants